Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[Doc] Start the tutorials for MKL-DNN backend #14202

Merged
merged 12 commits into from
Mar 19, 2019

Conversation

TaoLv
Copy link
Member

@TaoLv TaoLv commented Feb 19, 2019

Description

This is the beginning of a serial of PRs for improving the tutorials of MKL-DNN backend, including installation, design, usage, quantization, performance faq, etc.

This PR moved the MKLDNN_README file from root folder to the new tutorial folder and fixed some links according to the change.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@TaoLv TaoLv requested a review from szha as a code owner February 19, 2019 09:48
@TaoLv
Copy link
Member Author

TaoLv commented Feb 19, 2019

@pengzhao-intel
Copy link
Contributor

Thanks @TaoLv it will be very convenient for the end-user :)

@ZhennanQin please create INT8 doc in this folder after this PR merge.
@ciyongch please create RNN doc in this folder too

@pengzhao-intel
Copy link
Contributor

Because lot of outside pages reference the MKLDNN_README.
Could we keep the original page (maybe only link) to avoid 404?

@roywei
Copy link
Member

roywei commented Feb 21, 2019

@mxnet-label-bot add[Doc, Example, pr-awaiting-review]

@marcoabreu marcoabreu added Doc Example pr-awaiting-review PR is waiting for code review labels Feb 21, 2019
@TaoLv
Copy link
Member Author

TaoLv commented Feb 21, 2019

@aaronmarkham Could you help to review the change?

Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good start to improve the MKLDNN documentation :)

Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If MKL-DNN is on by default with the pip packages, then does it make sense to have the default build instructions for Windows/VS2017 to say that OpenBLAS is assumed? Why wouldn't the instructions be updated to bring some kind of conformity with what is suggested by what is provided by the pip package? To be specific, should these instructions for Windows be updated to suggest that MKL-DNN is used (at a minimum) if not also MKL?

@TaoLv
Copy link
Member Author

TaoLv commented Mar 1, 2019

@aaronmarkham Thank you for your kind suggestion. Currently, pip install mxnet doesn't have MKL-DNN enabled but mxnet-mkl does. Do you still think we need update the document to describe what's provided in mxnet-mkl and what's suggested for Windows build? @xinyu-intel

@TaoLv
Copy link
Member Author

TaoLv commented Mar 1, 2019

@aaronmarkham If yes, do you think we can do that in another PR? In this PR, I just try to move the document and make it visible on the web page, not intending to change any content in it.

@pengzhao-intel
Copy link
Contributor

friendly ping @aaronmarkham

@TaoLv
Copy link
Member Author

TaoLv commented Mar 8, 2019

Any comments? @aaronmarkham

@aaronmarkham
Copy link
Contributor

Sorry, I thought I already replied. We can do the docs update in a separate PR, but I wanted to flag that it should be done soon.

I was under the impression that MKL-DNN was on by default. I guess this must be in the build from source route then?

I ran into those subprocess issues. Should this be mentioned somewhere? Add a mention of KMP_INIT_AT_FORK=false - or maybe that needs to be thrown into the makefile to prevent problems?

Help me untangle what is what.
If you have already installed MKL and you pip install mxnet, what happens?
If you have already installed MKL-DNN and you pip install mxnet, what happens?
If you have already installed both MKL and MKL-DNN and you pip install mxnet, what happens?
If you have not installed either and you pip install mxnet-mkl, what happens?
If you have not installed either and you pip install mxnet, what happens?
If you have already installed MKL and you build from source (with default flags), what happens? I reported this issue where it doesn't work (on Windows): #11769
If you have already installed MKL-DNN and you build from source (with default flags), what happens?
What other variations pop up when we consider Apple or Linux or Edge?

@TaoLv
Copy link
Member Author

TaoLv commented Mar 11, 2019

I was under the impression that MKL-DNN was on by default. I guess this must be in the build from source route then?

Yes, MKL-DNN is on by default when build from source. For pypi package mxnet, it is explicitly turned off.

I ran into those subprocess issues. Should this be mentioned somewhere? Add a mention of KMP_INIT_AT_FORK=false - or maybe that needs to be thrown into the makefile to prevent problems?

I thought it's an issue of iomp not mxnet and someone is already working on it. I will track the progress of it. If it does not seem to be fixed soon, I will add it somewhere in the document.

If you have already installed MKL and you pip install mxnet, what happens?
If you have already installed MKL-DNN and you pip install mxnet, what happens?
If you have already installed both MKL and MKL-DNN and you pip install mxnet, what happens?

MXNet should run into openblas and native CPU implementation. MKL and MKL-DNN code is not compiled into mxnet pypi package - the code is wrapped with compiling flags MSHADOW_USE_MKL or MXNET_USE_MKLDNN.

If you have not installed either and you pip install mxnet-mkl, what happens?
If you have not installed either and you pip install mxnet, what happens?

mxnet-mkl/mxnet should work properly. It doesn't depend on any external installed package - MKL is statically linked and mkldnn.so is distributed with mxnet-mkl pypi package.

If you have already installed MKL and you build from source (with default flags), what happens? I reported this issue where it doesn't work (on Windows): #11769

You should explicitly use USE_BLAS=mkl to link MKL as BLAS library. USE_MKLDNN=1 should not affect the option choice. You issue looks like a cmake issue of mshadow to me.

If you have already installed MKL-DNN and you build from source (with default flags), what happens?

MXNet has its bundled MKL-DNN sub-module. The sub-module will be compiled and linked when USE_MKLDNN=1. External installed MKL-DNN package should not affect the process of build from source without some hack to the makefile or the link path.

What other variations pop up when we consider Apple or Linux or Edge?

Should share the similar methodology. One exception is that currently MKL-DNN is enabled by default only when build from source on linux + x86.

@TaoLv
Copy link
Member Author

TaoLv commented Mar 15, 2019

@aaronmarkham I have created #14399 to track the document improvement effort. Do you think this PR is good to merge and we can move forward now?

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: ) Thank you for your contribution!

@pengzhao-intel
Copy link
Contributor

@aaronmarkham do you have any other suggestion for our plan?
We're working on improving the MKLDNN documents.

If this is no other comment, I will merge this PR in 24 hours as the first step of doc enhancement.

@aaronmarkham
Copy link
Contributor

I think the separate issue to track the docs needs is great! Go ahead with this one.

@pengzhao-intel
Copy link
Contributor

Thanks, @aaronmarkham I am merging now :)

@pengzhao-intel pengzhao-intel merged commit c56c146 into apache:master Mar 19, 2019
@TaoLv
Copy link
Member Author

TaoLv commented Mar 19, 2019

Thank you @aaronmarkham . Feel free to comment in #14399 if you have any suggestion or question about MKL-DNN documentation.

vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
* start tutorials for mkldnn backend

* fix links

* add to tutorial index

* fix sanity test

* fix sanity test complain

* add to webpage

* Hint message and avoid 404
ZhennanQin pushed a commit to ZhennanQin/incubator-mxnet that referenced this pull request Apr 3, 2019
* start tutorials for mkldnn backend

* fix links

* add to tutorial index

* fix sanity test

* fix sanity test complain

* add to webpage

* Hint message and avoid 404
nswamy pushed a commit that referenced this pull request Apr 5, 2019
* start tutorials for mkldnn backend

* fix links

* add to tutorial index

* fix sanity test

* fix sanity test complain

* add to webpage

* Hint message and avoid 404
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* start tutorials for mkldnn backend

* fix links

* add to tutorial index

* fix sanity test

* fix sanity test complain

* add to webpage

* Hint message and avoid 404
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Doc Example pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants